-
Notifications
You must be signed in to change notification settings - Fork 190
Downgrade absence of sub- folders to warning #1928
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The legacy validator errored on the absence of subject directories, so this was written for consistency with that. I don't see the use-case for subject-less datasets. Calling a directory that contains opaque With my OpenNeuro hat on, removing basic "are there any data?" checks, combined with mechanisms to shield data from validation, is just asking for turn into an arbitrary data hosting service. So this is just a note that, if this is demoted to a warning, we will need to promote it back to an error, and should probably go ahead and promote it just in case. cc @nellh |
immediate, although not mandatory if we do add dedicated DatasetType:
But also it is a matter of consistency: if there is such a rule it should be clearly described also in the specification, which I think is not the case ATM. Indeed it might require openneuro specific configuration, but I thought you already have one anyways. |
I could be wrong but I think there is no strict and agreed upon requirement to require having sub- subfolders in BIDS specification. Although odd, thus warranting a warning, I think it is not strictly forbidden, and e.g. a BIDS dataset only with stimuli/ or derivatives/ or sourcedata might still be well legit BIDS dataset.
d3744b1 to
2f71c07
Compare
|
@arokem @barbarastrasser I see you gave thumbs ups on the initial post. Would either of you care to make a formal review? |
|
For the record, I agree with the argument that this error is not reflected in the text, so we should either update the text or relax the warning. OpenNeuro will continue to make it an error. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1928 +/- ##
=======================================
Coverage 82.65% 82.65%
=======================================
Files 17 17
Lines 1534 1534
=======================================
Hits 1268 1268
Misses 266 266 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Yes, this is great! This way we can remove the manual downgrade in the validator config :) |
I could be wrong but I think there is no strict and agreed upon requirement to require having sub- subfolders in BIDS specification. Although odd, thus warranting a warning, I think it is not strictly forbidden, and e.g. a BIDS dataset only with stimuli/ or derivatives/ or sourcedata might still be well legit BIDS dataset.
Check was added in b9ea963 likely just copying from prior node version of bids-validator. Asking @effigies for setting me straight ;)